Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Online DDL: ddl_strategy=direct #7172

Merged
merged 8 commits into from
Dec 22, 2020
Merged

Online DDL: ddl_strategy=direct #7172

merged 8 commits into from
Dec 22, 2020

Conversation

shlomi-noach
Copy link
Contributor

@shlomi-noach shlomi-noach commented Dec 14, 2020 •
edited
Loading

Backport

NO

Status

ready

Description

Following an internal discussion, where @askdba noticed this output:

 vtctlclient ApplySchema -sql="$(cat create_commerce_schema.sql)" commerce
Handling connection for 15999
I1213 17:07:05.817487   35734 main.go:64] I1213 14:07:05.661886 tablet_executor.go:259] Received DDL request. strategy =
I1213 17:07:05.882329   35734 main.go:64] I1213 14:07:05.729416 tablet_executor.go:259] Received DDL request. strategy =

strategy = (empty) seems confusing. Up till now, the empty string stood for the "direct" (non-online) DDL strategy.

Starting this PR, the dircet (non-online) strategy is named "direct". Consider the following examples:

mysql> select @@ddl_strategy;
+----------------+
| @@ddl_strategy |
+----------------+
| direct         |
+----------------+
1 row in set (0.00 sec)

mysql> alter table corder modify order_id int;
Query OK, 0 rows affected (0.03 sec)

mysql> set @@ddl_strategy='gh-ost';
Query OK, 0 rows affected (0.00 sec)

mysql> alter table corder modify order_id int;
+--------------------------------------+
| uuid                                 |
+--------------------------------------+
| 1305a8e4_3dd5_11eb_85ea_f875a4d24e90 |
+--------------------------------------+
1 row in set (0.00 sec)

mysql> set @@ddl_strategy='';
Query OK, 0 rows affected (0.00 sec)

mysql> alter table corder modify order_id int;
Query OK, 0 rows affected (0.01 sec)
/tmp/local$ vtctlclient ApplySchema -sql "alter table corder modify order_id int" commerce
I1214 08:25:23.239231 1630821 main.go:64] I1214 06:25:23.238050 tablet_executor.go:272] Received DDL request. strategy=direct
/tmp/local$ vtctlclient ApplySchema -ddl_strategy "gh-ost" -sql "alter table corder modify order_id int" commerce
I1214 08:25:38.917898 1630847 main.go:64] I1214 06:25:38.917187 tablet_executor.go:272] Received DDL request. strategy=gh-ost
I1214 08:25:38.918173 1630847 main.go:64] I1214 06:25:38.917839 tablet_executor.go:308] UUID=2ee751f1_3dd5_11eb_a6e5_f875a4d24e90
2ee751f1_3dd5_11eb_a6e5_f875a4d24e90
/tmp/local$ vtctlclient ApplySchema -ddl_strategy "" -sql "alter table corder modify order_id int" commerce
I1214 08:25:44.846210 1630860 main.go:64] I1214 06:25:44.844765 tablet_executor.go:272] Received DDL request. strategy=direct

notes:

  • in vtgate, the default value for -ddl_strategy command line flag is direct. An empty value is also interpreted as direct.
  • likewise, in vtctl/vtctlclient, the default value for -ddl_strategy command line flag is direct. An empty value is also interpreted as direct.

Related Issue(s)

Tracking: #6926

Todos

  • Tests
  • Documentation

Impacted Areas in Vitess

List general components of the application that this PR will affect:

  • Query Serving
  • VReplication
  • Cluster Management
  • Build

@shlomi-noach
Online DDL: ddl_strategy=direct
f3dcaab 
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
@shlomi-noach shlomi-noach mentioned this pull request Dec 14, 2020
Merged
@shlomi-noach
Copy link
Contributor Author

Documentation in vitessio/website#639

@shlomi-noach
Merge remote-tracking branch 'upstream/master' into ddl-strategy-direct
b65173c 
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
@askdba
Copy link
Contributor

askdba commented Dec 14, 2020

Did we intentionally used dd_strategy instead of ddl_strategy?

likewise, in vtctl/vtctlclient, the default value for -dd_strategy command line flag is direct. An empty value is also interpreted as direct.```

@shlomi-noach
Copy link
Contributor Author

Did we intentionally used dd_strategy instead of ddl_strategy?

That's just a typo in my comment. Fixed.

@shlomi-noach
merge master
d658771 
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
@askdba
Copy link
Contributor

askdba commented Dec 17, 2020

On the MySQL CLI I've got the same results as above test case.

Query OK, 0 rows affected (0.00 sec)

MySQL [commerce]> alter table corder modify order_id int;
+--------------------------------------+
| uuid                                 |
+--------------------------------------+
| fc1d1705_404a_11eb_b95d_0242ac110002 |
+--------------------------------------+
1 row in set (0.00 sec)

MySQL [commerce]>
MySQL [commerce]>
MySQL [commerce]> set @@ddl_strategy='direct';
Query OK, 0 rows affected (0.00 sec)

MySQL [commerce]> alter table corder modify order_id int;
Query OK, 0 rows affected (0.01 sec)

But on the command line, I did not get the same output.

vitess@5c644a6f4170:/vt/local$ vtctlclient ApplySchema -sql "alter table corder modify order_id bigint" commerce
vitess@5c644a6f4170:/vt/local$ vtctl --version
ERROR: logging before flag.Parse: E1217 09:36:03.895258    1618 syslogger.go:149] can't connect to syslog
Version: d65877152 (Git branch 'ddl-strategy-direct') built on Thu Dec 17 09:15:19 UTC 2020 by vitess@ecd12ae46f97 using go1.13.15 linux/amd64

@shlomi-noach
Copy link
Contributor Author

But on the command line, I did not get the same output.

That's because since this PR was opened, I already merged another PR that removes that log line altogether. If you'd like the log line to appear please let me know.

@askdba
Copy link
Contributor

askdba commented Dec 17, 2020

I've found the log message useful. I'd like it back.

@shlomi-noach
Merge remote-tracking branch 'upstream/master' into ddl-strategy-direct
6ef298a 
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
@shlomi-noach
restoring 'Received DDL request. strategy=' log message
ea5284a 
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
@shlomi-noach
Copy link
Contributor Author

Log message is now restored.

@shlomi-noach
Copy link
Contributor Author

🙏 request for additional review

ajm188
ajm188 approved these changes Dec 21, 2020
View reviewed changes
Copy link
Contributor

@ajm188 ajm188 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@shlomi-noach
onlineddl: init schema upon VExec request, which may come sooner than…
d0be367 
… first ticker

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
@shlomi-noach
reduce online ddl endtoend -migration_check_interval
526b741 
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
@shlomi-noach
fixed online ddl endtoend tests: need to parse strategy, or else it w…
5940f30 
…as 'direct' for strategy with options

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
@shlomi-noach shlomi-noach merged commit d4dedf4 into vitessio:master Dec 22, 2020
@shlomi-noach shlomi-noach deleted the ddl-strategy-direct branch December 22, 2020 10:48
@askdba askdba added this to the v9.0 milestone Dec 23, 2020
indera-shsp
indera-shsp reviewed Jan 26, 2021
View reviewed changes
go/vt/schema/online_ddl.go
// DDLStrategyGhost requests gh-ost to run the migration
DDLStrategyGhost DDLStrategy = "gh-ost"
// DDLStrategyPTOSC requests pt-online-schema-change to run the migration
DDLStrategyPTOSC DDLStrategy = "pt-osc"
)

// IsDirect returns true if this strategy is a direct strategy
// A strategy is direct if it's not explciitly one of the online DDL strategies
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit pick: explciitly

indera-shsp
indera-shsp reviewed Jan 26, 2021
View reviewed changes
go/vt/schemamanager/tablet_executor.go
@@ -232,6 +232,7 @@ func (exec *TabletExecutor) executeSQL(ctx context.Context, sql string, execResu
return nil
}
}
exec.wr.Logger().Infof("Received DDL request. strategy=%+v", schema.DDLStrategyDirect)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps it is useful to log the sql too 🤷‍♂️

@rafael rafael mentioned this pull request Mar 17, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@indera-shsp indera-shsp indera-shsp left review comments

@ajm188 ajm188 ajm188 approved these changes

@deepthi deepthi Awaiting requested review from deepthi

@harshit-gangal harshit-gangal Awaiting requested review from harshit-gangal

@sougou sougou Awaiting requested review from sougou

@systay systay Awaiting requested review from systay

@askdba askdba Awaiting requested review from askdba

@doeg doeg Awaiting requested review from doeg

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v9.0
Development

Successfully merging this pull request may close these issues.
4 participants
@shlomi-noach @askdba @ajm188 @indera-shsp